xds: refactor ADS related data to xDS-Manager part 1 #38145
Conversation
Signed-off-by: Adi Suissa-Peleg <[email protected]>
|
/assign @nezdolik |
| * @param cm - a pointer to a valid cluster manager. | ||
| * @return Ok if the initialization was successful, or an error otherwise. | ||
| */ | ||
| virtual absl::Status initialize(Upstream::ClusterManager* cm) PURE; |
There was a problem hiding this comment.
should the param be a reference type instead? E.g. this way the api would imply you can only use it with a valid cm instance
There was a problem hiding this comment.
I'm open to passing a reference instead of pointer (if it makes sense).
Nevertheless, the XdsManagerImpl class will need to hold a pointer to the cluster-manager and not a reference, as the xds-manager is instantiated before the cluster-manager.
LMK if you still think we should have this argument to a reference, and if so, I'll make the change.
There was a problem hiding this comment.
in that case everything looks good, i was under impression that cm is constructed before the xds manager.
|
|
||
| // Config::ConfigSourceProvider | ||
| absl::Status initialize(Upstream::ClusterManager* cm) override; | ||
| void shutdown() override {} |
There was a problem hiding this comment.
will this be implemented later?
There was a problem hiding this comment.
Yes, this will shutdown the ADS once ads_mux_ is moved from the cluster-manager to the xds-manager (see here.
The reason I'm introducing the method at this stage is to reduce the amount of changes in the next refactors (smaller PRs).
|
|
||
| // Config::ConfigSourceProvider | ||
| absl::Status initialize(Upstream::ClusterManager* cm) override; | ||
| void shutdown() override {} |
There was a problem hiding this comment.
Yes, this will shutdown the ADS once ads_mux_ is moved from the cluster-manager to the xds-manager (see here.
The reason I'm introducing the method at this stage is to reduce the amount of changes in the next refactors (smaller PRs).
| * @param cm - a pointer to a valid cluster manager. | ||
| * @return Ok if the initialization was successful, or an error otherwise. | ||
| */ | ||
| virtual absl::Status initialize(Upstream::ClusterManager* cm) PURE; |
There was a problem hiding this comment.
I'm open to passing a reference instead of pointer (if it makes sense).
Nevertheless, the XdsManagerImpl class will need to hold a pointer to the cluster-manager and not a reference, as the xds-manager is instantiated before the cluster-manager.
LMK if you still think we should have this argument to a reference, and if so, I'll make the change.
Signed-off-by: Adi Suissa-Peleg <[email protected]>
|
/assign-from @envoyproxy/senior-maintainers |
|
@envoyproxy/senior-maintainers assignee is @RyanTheOptimist |
Commit Message: xds: refactor ADS related data to xDS-Manager part 1 Additional Description: As part of the work on envoyproxy#24773 the xDS-related functionality needs to be moved from the cluster-manager to the xDS-manager. This is the first refactor: moving the xDS-manager initialization to the end of the cluster-manager's c'tor. Risk Level: low Testing: Updated tests. Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Adi Suissa-Peleg <[email protected]>
- synced changes to `.bazelrc`, `.bazelversion`, `tools/code_format/config.yaml` from Envoy's version. - no changes in `run_envoy_docker.sh`, `tools/gen_compilation_database.py`. - added an instance of `Envoy::Config::XdsManager&` onto the `NighthawkServerFactoryContext`, since the `ClusterManager` constructor now calls this new method unconditionally. Nothing in Nighthawk will use the `Envoy::Config::XdsManager&` instance. This is due to envoyproxy/envoy#38145. Signed-off-by: Jakub Sobon <[email protected]>
- synced changes to `.bazelrc`, `.bazelversion`, `tools/code_format/config.yaml` from Envoy's version. - no changes in `run_envoy_docker.sh`, `tools/gen_compilation_database.py`. - added an instance of `Envoy::Config::XdsManager&` onto the `NighthawkServerFactoryContext`, since the `ClusterManager` constructor now calls this new method unconditionally. Nothing in Nighthawk will use the `Envoy::Config::XdsManager&` instance. This is due to envoyproxy/envoy#38145. - tune the open loop mini stress test whjich started being flaky under `test_gcc`. Ideally we shouldn't be running stress test on CI at all, but leaving it in as long as it is able to pass. - no update needed in `tools/base/requirements.in`. Signed-off-by: Jakub Sobon <[email protected]>
Commit Message: xds: refactor ADS related data to xDS-Manager part 1
Additional Description:
As part of the work on #24773 the xDS-related functionality needs to be moved from the cluster-manager to the xDS-manager.
This is the first refactor: moving the xDS-manager initialization to the end of the cluster-manager's c'tor.
Risk Level: low
Testing: Updated tests.
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A